chore: swap mypy with pyrefly for type checking #817
Conversation
- Replace mypy dependency with pyrefly (>=0.25.0) - Remove pytest-mypy (mypy-specific dependency) - Convert [tool.mypy] configuration to [tool.pyrefly] - Configure pyrefly to match mypy's lenient behavior by disabling strict error kinds - Update CI workflow from mypy-check to pyrefly-check - Update poe check task to use 'pyrefly check' instead of 'mypy .' - Update test_mypy.py to run pyrefly checks Pyrefly is a faster type checker from Meta that provides similar functionality to mypy. The configuration disables 61 strict error kinds to maintain compatibility with the existing codebase while allowing for gradual re-enablement to improve type safety over time. All checks pass: ruff formatting/linting, pyrefly type checking (0 errors), pytest collection. Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1759466150-swap-mypy-with-pyrefly' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1759466150-swap-mypy-with-pyrefly'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
.github/workflows/python_lint.yml
Outdated
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 11 days ago
To fix the problem, we should explicitly add a permissions key with the correct minimal privileges required for this workflow. Since all jobs in this workflow only need to check out code and run local linting/formatting commands, they only require read access to the repository contents. The fix can be implemented by adding permissions: contents: read at the root level of the workflow, ensuring the minimal privileges are granted for all jobs. No step in any job requires broader permissions. This change should be added above the jobs: key (line 12), preferably near the top of the YAML file for visibility.
| @@ -9,6 +9,8 @@ | ||
| env: | ||
| AIRBYTE_ANALYTICS_ID: ${{ vars.AIRBYTE_ANALYTICS_ID }} | ||
|
|
||
| permissions: | ||
| contents: read | ||
| jobs: | ||
| ruff-lint-check: | ||
| name: Ruff Lint Check |
📝 WalkthroughWalkthroughReplaces MyPy with Pyrefly across CI, pyproject configuration, dev dependencies, and lint tests; adds pyrefly lint annotations and minor formatting/annotation edits across multiple modules; changes a generator to yield (AirbyteMessage, Path). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Reader as FileReader
participant Decoder as JSONDecoder
participant Consumer as MessageConsumer
Reader->>Decoder: read line from file (next_line)
Decoder->>Decoder: parse JSON -> AirbyteMessage
Decoder-->>Consumer: yield (AirbyteMessage, Path) %% new: yields tuple with path
Note right of Consumer: Consumer receives message + source path\n(previously only message)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Would you like to add other reviewers or prioritize any specific files for deeper review? wdyt? Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/python_lint.yml (1)
61-85: Consider adding explicit permissions to the workflow job.The workflow job looks good and correctly migrates from mypy to pyrefly. However, as flagged by CodeQL, consider adding an explicit
permissionsblock for security best practice, wdyt?Apply this diff to add minimal permissions:
pyrefly-check: name: Pyrefly Check runs-on: ubuntu-latest + permissions: + contents: read steps:Based on static analysis hints.
tests/lint_tests/test_mypy.py (1)
10-23: LGTM! Consider renaming the test function?The command and messages are correctly updated to use pyrefly. The test logic remains sound.
Minor note: the function name
test_mypy_typingstill references mypy. Would you like to rename it totest_pyrefly_typingfor consistency, or keep it as-is for backwards compatibility, wdyt?pyproject.toml (1)
133-151: Plan to incrementally re-enable these error kinds?The 16 disabled error kinds are well-documented as part of the gradual migration strategy from mypy. This is a pragmatic approach given that pyrefly found 61 new errors.
Consider tracking these suppressions in a follow-up issue or roadmap to ensure they're systematically re-enabled over time to improve type safety, wdyt?
Do you want me to open a new issue to track the incremental re-enabling of these error kinds?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/python_lint.yml(2 hunks).gitignore(1 hunks)pyproject.toml(3 hunks)tests/lint_tests/test_mypy.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/python_lint.yml
[warning] 62-85: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (4)
.gitignore (1)
121-121: LGTM!The header comment accurately reflects the migration to pyrefly while keeping the original mypy references for backwards compatibility with the existing ignore patterns.
pyproject.toml (3)
64-64: LGTM!The pyrefly dependency is correctly added with an appropriate version constraint.
119-132: LGTM!The pyrefly configuration correctly migrates the old mypy settings. The
python-version,project-includes,project-excludes, and behavior flags appropriately preserve the previous type-checking scope and semantics.
173-173: LGTM!The
checktask correctly updates from mypy to pyrefly, maintaining the same workflow structure with the new tool.
- Remove [tool.pyrefly.errors] section from pyproject.toml - Add inline # pyrefly: ignore[error-kind] comments for 36 legitimate type variances - Remove 2 redundant casts in cloud/connections.py (pyrefly infers types correctly) - Add explicit imports for sqlalchemy.exc and google.auth (avoid implicit imports) All pyrefly checks now pass with 0 errors using targeted inline suppressions instead of global suppressions. Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
airbyte/cloud/connections.py (1)
97-128: Keep returning strings for IDs to avoid JSON serialization failuresLines 105 and 128 now hand back whichever type the API client gives us. After lazy-loading,
ConnectionResponse.source_id/destination_idarrive asuuid.UUIDobjects, so passingself.source_idinto helpers likeCloudWorkspace.permanently_delete_source()ends up feeding aUUIDintorequests’ JSON encoder, which raisesTypeError: Object of type UUID is not JSON serializable. Restoring the explicitstr()coercion preserves the previous, working runtime behavior.Could we keep the string conversion like below, wdyt?
- return self._source_id + return str(self._source_id) ... - return self._destination_id + return str(self._destination_id)airbyte/shared/state_providers.py (2)
59-70: Fix the return type annotation instead of suppressing the error.The
state_message_artifactsproperty is annotated to returnIterable[AirbyteStreamState](Line 61), but it actually returns_state_message_artifacts, which is typed asIterable[AirbyteStateMessage](Line 33). This is a genuine type mismatch that pyrefly caught. Instead of ignoring the error, consider updating the return type annotation on Line 61 toIterable[AirbyteStateMessage]to match the implementation, wdyt?Apply this diff to fix the return type annotation:
@property def state_message_artifacts( self, - ) -> Iterable[AirbyteStreamState]: + ) -> Iterable[AirbyteStateMessage]: """Return all state artifacts. This is just a type guard around the private variable `_state_message_artifacts`. """ result = self._state_message_artifacts if result is None: raise exc.PyAirbyteInternalError(message="No state artifacts were declared.") - return result # pyrefly: ignore[bad-return] + return result
95-116: Refine the function signature or add a runtime check.The
get_stream_statemethod is annotated to returnAirbyteStateMessage(Line 100), but whennot_foundisNone, the function would returnNone(Line 110), which is a type error. The suppression hides this mismatch. Consider one of these approaches, wdyt?
- Option 1: Update the return type to
AirbyteStateMessage | Noneto reflect thatNonecan be returned whennot_found=None.- Option 2: Add a runtime check before Line 110 to ensure
not_foundis notNone, and raise an error if it is.Option 1: Update the return type annotation:
def get_stream_state( self, /, stream_name: str, not_found: AirbyteStateMessage | Literal["raise"] | None = "raise", - ) -> AirbyteStateMessage: + ) -> AirbyteStateMessage | None: """Return the state message for the specified stream name.""" for state_message in self.state_message_artifacts: if ( state_message.stream.stream_descriptor.name # pyrefly: ignore[missing-attribute] == stream_name ): return state_message # pyrefly: ignore[bad-return] if not_found != "raise": - return not_found # pyrefly: ignore[bad-return] + return not_found raise exc.AirbyteStateNotFoundError( message="State message not found.", stream_name=stream_name, available_streams=list(self.known_stream_names), )Option 2: Add a runtime check (if returning
Noneis not intended):if not_found != "raise": + if not_found is None: + raise exc.PyAirbyteInternalError( + message="`not_found` cannot be None when not raising." + ) - return not_found # pyrefly: ignore[bad-return] + return not_found
🧹 Nitpick comments (2)
pyproject.toml (1)
64-64: Consider tightening the pyrefly version constraint.The pyrefly dependency uses
>=0.25.0without an upper bound, which might lead to unexpected breaking changes in future releases. Consider using a tighter constraint like^0.25.0(which is equivalent to>=0.25.0,<0.26.0) to prevent breaking changes, wdyt?Apply this diff to tighten the version constraint:
-pyrefly = ">=0.25.0" +pyrefly = "^0.25.0"airbyte/_util/name_normalizers.py (1)
51-53: Keep suppression and open Pyrefly issue?I verified that
@staticmethod+@functools.cacheis correctly ordered in both name_normalizers.py and postgres.py, so the[bad-override]warning appears to be a Pyrefly limitation. Shall we retain the# pyrefly: ignore[bad-override]here and file an upstream issue to track it? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
airbyte/_connector_base.py(1 hunks)airbyte/_message_iterators.py(2 hunks)airbyte/_processors/sql/bigquery.py(1 hunks)airbyte/_processors/sql/postgres.py(1 hunks)airbyte/_processors/sql/snowflake.py(1 hunks)airbyte/_util/api_util.py(2 hunks)airbyte/_util/meta.py(1 hunks)airbyte/_util/name_normalizers.py(1 hunks)airbyte/caches/_catalog_backend.py(1 hunks)airbyte/caches/base.py(1 hunks)airbyte/caches/motherduck.py(1 hunks)airbyte/cloud/connections.py(3 hunks)airbyte/cloud/sync_results.py(2 hunks)airbyte/datasets/_sql.py(1 hunks)airbyte/destinations/_translate_cache_to_dest.py(1 hunks)airbyte/mcp/local_ops.py(1 hunks)airbyte/progress.py(2 hunks)airbyte/secrets/google_gsm.py(1 hunks)airbyte/shared/catalog_providers.py(3 hunks)airbyte/shared/sql_processor.py(2 hunks)airbyte/shared/state_providers.py(2 hunks)airbyte/sources/base.py(1 hunks)airbyte/types.py(1 hunks)pyproject.toml(3 hunks)
✅ Files skipped from review due to trivial changes (12)
- airbyte/types.py
- airbyte/sources/base.py
- airbyte/caches/base.py
- airbyte/_util/api_util.py
- airbyte/progress.py
- airbyte/datasets/_sql.py
- airbyte/shared/catalog_providers.py
- airbyte/mcp/local_ops.py
- airbyte/_connector_base.py
- airbyte/caches/motherduck.py
- airbyte/caches/_catalog_backend.py
- airbyte/destinations/_translate_cache_to_dest.py
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/_util/name_normalizers.py (1)
airbyte/_processors/sql/postgres.py (1)
normalize(54-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (10)
airbyte/_util/meta.py (1)
137-137: Inconsistency between AI summary and code—pyrefly directive is correct.The AI summary mentions
pyrefly: ignore[missing-attribute], but the actual code usespyrefly: ignore[import-error]. The latter is correct for suppressing warnings about the optional IPython import. The directive is appropriate since IPython is not a required dependency and the import is already guarded bysuppress(Exception). Just noting the summary discrepancy—wdyt?airbyte/_processors/sql/snowflake.py (1)
207-209: Aligntype_converter_classannotation with parent or document suppression
The parentSqlProcessorBase.type_converter_classis declared astype[SQLTypeConverter](airbyte/shared/sql_processor.py:174), but here it’s annotated astype[SnowflakeTypeConverter], triggeringbad-override. Should we update this totype[SQLTypeConverter]to satisfy the override or keep the suppression? wdyt?airbyte/_processors/sql/postgres.py (1)
74-75: Inline ignore looks consistentLine 74 keeps the existing runtime behavior while documenting the pyrefly override; thanks for staying consistent with the other processors.
airbyte/secrets/google_gsm.py (1)
61-62: Annotation tweak is spot onLine 61’s inline ignore mirrors the pattern elsewhere and keeps the handle metadata unchanged—looks great.
airbyte/_processors/sql/bigquery.py (1)
10-99: New imports line up with usageBringing in
google.authandsqlalchemy.excmatches the credential fallback and warning filter in Lines 73–99—nice tidy-up.airbyte/cloud/sync_results.py (1)
205-471: Pyrefly ignores keep the formatter untouchedLines 205–471 only add the inline ignores, leaving the log output and stream accessor behavior the same—looks good.
airbyte/shared/sql_processor.py (1)
16-16: LGTM!Explicitly importing
sqlalchemy.excat the top is good practice, especially since it's used later in the file (lines 828-829). This makes the dependency clear.pyproject.toml (3)
133-136: Clarify the intent of the[tool.pyright]configuration.The addition of a
[tool.pyright]section suggests that pyright is being configured alongside pyrefly. Please clarify:
- Is pyright intended to be used as an additional type checker in CI/development?
- Or is this configuration just for IDE support (e.g., VS Code's Pylance)?
If pyright is only for IDE support, consider adding a comment in the configuration to clarify this, wdyt?
153-153: LGTM!The check command has been updated to run
pyrefly checkinstead ofmypy, which aligns with the migration from mypy to pyrefly. This change is consistent with the PR objectives.
119-132: Verify[tool.pyrefly.errors]configuration.I don’t see a
[tool.pyrefly.errors]section in pyproject.toml; can you confirm it’s included, lists the 16 disabled error kinds, and that there’s a documented plan to re-enable them incrementally? wdyt?
| yield ( # pyrefly: ignore[invalid-yield] | ||
| AirbyteMessage.model_validate_json(next_line), | ||
| current_file, | ||
| ) |
There was a problem hiding this comment.
Blocker: Yielding a tuple breaks the iterator contract
AirbyteMessageIterator promises Iterator[AirbyteMessage], but this generator now yields (AirbyteMessage, Path). Every consumer will start receiving tuples instead of messages, so iteration, .read(), etc., will misbehave. Could we keep yielding a single AirbyteMessage, or (if the caller truly needs the path) expand the iterator’s type and update all call sites to handle the tuple? wdyt?
🤖 Prompt for AI Agents
In airbyte/_message_iterators.py around lines 197 to 200 the generator yields a
tuple (AirbyteMessage, Path) which violates the declared
Iterator[AirbyteMessage]; change the generator to yield only the AirbyteMessage
(i.e., yield AirbyteMessage.model_validate_json(next_line)) so consumers still
receive the expected type, and if the file path is required by callers either
(a) attach it to the AirbyteMessage via a new optional attribute before yielding
or (b) create a new iterator type that returns tuples and update all call sites
and type annotations accordingly—prefer option (a) for minimal changes and
preserve existing API.
| raise exc.AirbyteInternalError( # pyrefly: ignore[missing-attribute] | ||
| message="Invalid `write_strategy` argument. Expected instance of WriteStrategy.", | ||
| context={"write_strategy": write_strategy}, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the [missing-attribute] suppression on AirbyteInternalError.
Pyrefly is flagging a [missing-attribute] error on the AirbyteInternalError exception (lines 277-280). This could indicate that:
- The exception class doesn't support the
contextargument, or - Pyrefly's type stubs are incomplete for the exception class.
Consider verifying that AirbyteInternalError supports the message and context arguments, and whether pyrefly is correctly configured to recognize the exception class. If this is a false positive, it might be worth reporting to the pyrefly maintainers, wdyt?
Run the following script to verify the AirbyteInternalError signature:
🏁 Script executed:
#!/bin/bash
# Description: Verify the signature of AirbyteInternalError.
# Search for the class definition of AirbyteInternalError
rg -nP --type=py -C5 'class AirbyteInternalError'Length of output: 0
Could we locate the definition of AirbyteInternalError across the repo to inspect its constructor signature? wdyt?
🏁 Script executed:
#!/bin/bash
# Locate references and definition of AirbyteInternalError
rg -n 'AirbyteInternalError' -n .Length of output: 3332
Could we locate the AirbyteInternalError class definition in airbyte/exceptions.py to confirm its __init__ signature (i.e., support for message and context)? wdyt?
🏁 Script executed:
#!/bin/bash
# Search for exception class definitions ending with InternalError
rg -nP 'class .*InternalError' -n airbyte/exceptions.pyLength of output: 49
Undefined exception AirbyteInternalError (airbyte/shared/sql_processor.py:277)
exc.AirbyteInternalError isn’t defined in airbyte.exceptions. Did you mean to use exc.PyAirbyteInternalError? wdyt?
🤖 Prompt for AI Agents
In airbyte/shared/sql_processor.py around lines 277 to 280, the code raises
exc.AirbyteInternalError which is not defined in airbyte.exceptions; change the
raised exception to the correct class (exc.PyAirbyteInternalError) or import the
proper AirbyteInternalError symbol from the module that defines it so the name
exists; update the raise to use exc.PyAirbyteInternalError (or adjust imports)
and run tests to ensure the updated exception is recognized.
| if ( | ||
| state_message.stream.stream_descriptor.name # pyrefly: ignore[missing-attribute] | ||
| == stream_name | ||
| ): | ||
| return state_message # pyrefly: ignore[bad-return] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
These suppressions should become unnecessary after fixing state_message_artifacts.
Once the return type of state_message_artifacts is corrected to Iterable[AirbyteStateMessage] (as suggested in the previous comment), pyrefly should be able to verify that state_message.stream exists and that returning state_message is type-safe. Consider removing the # pyrefly: ignore[...] comments here after that fix, wdyt?
🤖 Prompt for AI Agents
In airbyte/shared/state_providers.py around lines 103 to 107, the two pyrefly
ignore comments are masking a typing mismatch caused by state_message_artifacts
having an incorrect return type; update state_message_artifacts to return
Iterable[AirbyteStateMessage] (and adjust any imports/annotations) so pyrefly
can infer that state_message has a .stream attribute and that returning
state_message is type-safe, then remove the "# pyrefly:
ignore[missing-attribute]" and "# pyrefly: ignore[bad-return]" comments from
these lines.
1 participant
chore: swap mypy with pyrefly for type checking
Summary
This PR replaces mypy with pyrefly (Meta's faster type checker) across the entire PyAirbyte codebase. The changes include:
mypy ^1.11.2withpyrefly >=0.25.0and removedpytest-mypy[tool.mypy]section to[tool.pyrefly]with equivalent settingsmypy-checktopyrefly-checkpoe checktask and test files to usepyrefly checkKey Configuration Decision: Pyrefly is stricter than mypy by default and initially found 61 type errors that mypy wasn't catching. To maintain compatibility during the migration, the configuration disables 16 error kinds that can be incrementally re-enabled to improve type safety over time.
Review & Testing Checklist for Human (4 items - HIGH RISK)
poetry install && poetry run pyrefly checkto ensure the tool works without errorspyrefly-checkCI job passes[tool.pyrefly.errors]to determine if the suppression is too aggressive for the team's type safety standardspoe test-fastto ensure no integration issues with the new type checkerNotes
Summary by CodeRabbit